Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[feat](cloud) S3 storage vault support property use_path_style #43060

Merged
merged 1 commit into from
Nov 6, 2024
Merged

[feat](cloud) S3 storage vault support property use_path_style #43060

merged 1 commit into from
Nov 6, 2024

Conversation

SWJTU-ZhangLei
Copy link
Contributor

@SWJTU-ZhangLei SWJTU-ZhangLei commented Nov 1, 2024 •
edited by gavinchou
Loading

example

  CREATE STORAGE VAULT IF NOT EXISTS demo_vault
    PROPERTIES (
      "type"="S3",
      ...
      "use_path_style" = "true"
  );

doc apache/doris-website#1344

@doris-robot
Copy link

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR

Since 2024-03-18, the Document has been moved to doris-website.
See Doris Document.

@SWJTU-ZhangLei
Copy link
Contributor Author

run buildall

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 1, 2024

clang-tidy review says "All clean, LGTM! 👍"

@doris-robot
Copy link

TeamCity be ut coverage result:
Function Coverage: 37.82% (9820/25968)
Line Coverage: 28.98% (81577/281502)
Region Coverage: 28.26% (42144/149105)
Branch Coverage: 24.83% (21374/86070)
Coverage Report: http://coverage.selectdb-in.cc/coverage/261f97943bf7764d120d19caa554670d41aaff37_261f97943bf7764d120d19caa554670d41aaff37/report/index.html

dataroaring
dataroaring previously approved these changes Nov 1, 2024
View reviewed changes
Copy link
Contributor

@dataroaring dataroaring left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@github-actions github-actions bot added the approved Indicates a PR has been approved by one committer. label Nov 1, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 1, 2024

PR approved by at least one committer and no changes requested.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 1, 2024

PR approved by anyone and no changes requested.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 1, 2024

clang-tidy review says "All clean, LGTM! 👍"

gavinchou
gavinchou reviewed Nov 1, 2024
View reviewed changes
fe/fe-core/src/main/java/org/apache/doris/datasource/property/constants/S3Properties.java Outdated
@@ -334,6 +334,11 @@ public static Cloud.ObjectStoreInfoPB.Builder getObjStoreInfoPB(Map<String, Stri
// S3 Provider properties should be case insensitive.
builder.setProvider(Provider.valueOf(properties.get(S3Properties.PROVIDER).toUpperCase()));
}

if (properties.containsKey(PropertyConverter.USE_PATH_STYLE)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can it be modified by alter vault stmt?

Copy link
Contributor Author

@SWJTU-ZhangLei SWJTU-ZhangLei Nov 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can it be modified by alter vault stmt?

done

@SWJTU-ZhangLei
Copy link
Contributor Author

run buildall

@github-actions github-actions bot removed the approved Indicates a PR has been approved by one committer. label Nov 4, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 4, 2024

clang-tidy review says "All clean, LGTM! 👍"

@doris-robot
Copy link

TeamCity be ut coverage result:
Function Coverage: 37.80% (9819/25974)
Line Coverage: 28.98% (81660/281810)
Region Coverage: 28.21% (42094/149194)
Branch Coverage: 24.80% (21361/86140)
Coverage Report: http://coverage.selectdb-in.cc/coverage/6d7486e2d4b0ffca6842b076495c7a9cc0f0b5e6_6d7486e2d4b0ffca6842b076495c7a9cc0f0b5e6/report/index.html

@SWJTU-ZhangLei
Copy link
Contributor Author

run buildall

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 4, 2024

clang-tidy review says "All clean, LGTM! 👍"

@doris-robot
Copy link

TeamCity be ut coverage result:
Function Coverage: 37.81% (9822/25975)
Line Coverage: 28.99% (81711/281818)
Region Coverage: 28.23% (42121/149203)
Branch Coverage: 24.81% (21375/86144)
Coverage Report: http://coverage.selectdb-in.cc/coverage/ac77d1b29afb175eb61f09e22750a1158dd9fab2_ac77d1b29afb175eb61f09e22750a1158dd9fab2/report/index.html

gavinchou
gavinchou reviewed Nov 4, 2024
View reviewed changes
fe/fe-core/src/main/java/org/apache/doris/datasource/property/constants/S3Properties.java Outdated
String value = properties.get(PropertyConverter.USE_PATH_STYLE);
Preconditions.checkArgument(!Strings.isNullOrEmpty(value), "use_path_style cannot be empty");
Preconditions.checkArgument(value.equalsIgnoreCase("true")
|| value.equalsIgnoreCase("false"), "Invalid use_path_style value: %s", value);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Invalid use_path_style value: %s, only 'true' or 'false' is acceptable"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Invalid use_path_style value: %s, only 'true' or 'false' is acceptable"

resolved

gavinchou
gavinchou reviewed Nov 4, 2024
View reviewed changes
fe/fe-core/src/main/java/org/apache/doris/datasource/property/constants/S3Properties.java Outdated
Comment on lines 344 to 349
if (properties.get(PropertyConverter.USE_PATH_STYLE).equalsIgnoreCase("true")) {
builder.setUsePathStyle(true);
} else {
builder.setUsePathStyle(false);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (properties.get(PropertyConverter.USE_PATH_STYLE).equalsIgnoreCase("true")) {
builder.setUsePathStyle(true);
} else {
builder.setUsePathStyle(false);
}
builder.setUsePathStyle(value.equalsIgnoreCase("true"));

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe alter set use_path_style from true to false

@SWJTU-ZhangLei
Copy link
Contributor Author

run buildall

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 5, 2024

clang-tidy review says "All clean, LGTM! 👍"

@SWJTU-ZhangLei
Copy link
Contributor Author

run buildall

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 5, 2024

PR approved by at least one committer and no changes requested.

@doris-robot
Copy link

TeamCity be ut coverage result:
Function Coverage: 37.79% (9819/25981)
Line Coverage: 28.96% (81638/281927)
Region Coverage: 28.21% (42099/149239)
Branch Coverage: 24.79% (21357/86164)
Coverage Report: http://coverage.selectdb-in.cc/coverage/1ff0dd6ee60f4c0c32f1237f8259e77d5de5aa2d_1ff0dd6ee60f4c0c32f1237f8259e77d5de5aa2d/report/index.html

@SWJTU-ZhangLei
Copy link
Contributor Author

run buildall

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 5, 2024

clang-tidy review says "All clean, LGTM! 👍"

@SWJTU-ZhangLei
Copy link
Contributor Author

run buildall

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 5, 2024

clang-tidy review says "All clean, LGTM! 👍"

@SWJTU-ZhangLei
[feat](cloud) S3 storage vault support property use_path_style
b5f9d28 
```
  CREATE STORAGE VAULT IF NOT EXISTS demo_vault
    PROPERTIES (
      "type"="S3",
      ...
      "use_path_style" = "true"
  );

```
@SWJTU-ZhangLei
Copy link
Contributor Author

run buildall

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 6, 2024

clang-tidy review says "All clean, LGTM! 👍"

@doris-robot
Copy link

TeamCity be ut coverage result:
Function Coverage: 37.86% (9837/25982)
Line Coverage: 29.03% (81810/281850)
Region Coverage: 28.27% (42167/149164)
Branch Coverage: 24.86% (21402/86100)
Coverage Report: http://coverage.selectdb-in.cc/coverage/b5f9d28d0a63ad0fa6f863add6088fa625a2d2d2_b5f9d28d0a63ad0fa6f863add6088fa625a2d2d2/report/index.html

@SWJTU-ZhangLei
Copy link
Contributor Author

run p0

@SWJTU-ZhangLei
Copy link
Contributor Author

run cloud_p0

@SWJTU-ZhangLei
Copy link
Contributor Author

run p0

dataroaring
dataroaring approved these changes Nov 6, 2024
View reviewed changes
Copy link
Contributor

@dataroaring dataroaring left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dataroaring dataroaring merged commit 4a7b504 into apache:master Nov 6, 2024
25 of 27 checks passed
github-actions bot pushed a commit that referenced this pull request Nov 6, 2024
@SWJTU-ZhangLei
[feat](cloud) S3 storage vault support property use_path_style (#43060
85ac2e6 
)

example 
```
  CREATE STORAGE VAULT IF NOT EXISTS demo_vault
    PROPERTIES (
      "type"="S3",
      ...
      "use_path_style" = "true"
  );
```
@github-actions github-actions bot mentioned this pull request Nov 6, 2024
Merged
SWJTU-ZhangLei added a commit to SWJTU-ZhangLei/incubator-doris that referenced this pull request Nov 6, 2024
@SWJTU-ZhangLei
[feat](cloud) S3 storage vault support property use_path_style (apa…
8313cdb 
…che#43060)

example 
```
  CREATE STORAGE VAULT IF NOT EXISTS demo_vault
    PROPERTIES (
      "type"="S3",
      ...
      "use_path_style" = "true"
  );
```
@SWJTU-ZhangLei SWJTU-ZhangLei mentioned this pull request Nov 6, 2024
Merged
16 tasks
dataroaring pushed a commit that referenced this pull request Nov 7, 2024
@github-actions @SWJTU-ZhangLei
branch-3.0: [feat](cloud) S3 storage vault support property `use_path…
3a47e3d 
…_style` (#43330)

Cherry-picked from #43060

Co-authored-by: Lei Zhang <27994433+SWJTU-ZhangLei@users.noreply.github.com>
gavinchou pushed a commit that referenced this pull request Nov 8, 2024
@SWJTU-ZhangLei
[feat](cloud) S3 storage vault support property use_path_style (#43060
16a83fb 
) (#43343)

example 
```
  CREATE STORAGE VAULT IF NOT EXISTS demo_vault
    PROPERTIES (
      "type"="S3",
      ...
      "use_path_style" = "true"
  );
```
@gavinchou gavinchou added cir and removed approved Indicates a PR has been approved by one committer. labels Nov 11, 2024
@SWJTU-ZhangLei SWJTU-ZhangLei mentioned this pull request Nov 13, 2024
Merged
4 tasks
@gavinchou gavinchou mentioned this pull request Nov 26, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gavinchou gavinchou gavinchou approved these changes

@Yukang-Lian Yukang-Lian Yukang-Lian approved these changes

@dataroaring dataroaring dataroaring approved these changes

Assignees
No one assigned
Labels
cir dev/3.0.3-merged p0_b reviewed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@SWJTU-ZhangLei @doris-robot @gavinchou @Yukang-Lian @dataroaring